ROX-30296: track POSIX ACL changes via inode_set_acl LSM hook#878
ROX-30296: track POSIX ACL changes via inode_set_acl LSM hook#878Stringy wants to merge 12 commits into
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (16)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #878 +/- ##
==========================================
- Coverage 32.23% 31.17% -1.06%
==========================================
Files 21 21
Lines 2736 2829 +93
Branches 2736 2829 +93
==========================================
Hits 882 882
- Misses 1851 1944 +93
Partials 3 3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
46d84a9 to
11683ba
Compare
67a3b12 to
be7fb22
Compare
eae45f0 to
fc73ed7
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fact/src/event/mod.rs (1)
538-555: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd the missing
Chownequality arm.While extending this manual matcher,
FileData::Chownstill falls through to_ => false, so two identical ownership-change events compare unequal.Proposed fix
(FileData::Unlink(this), FileData::Unlink(other)) => this == other, (FileData::Chmod(this), FileData::Chmod(other)) => this == other, + (FileData::Chown(this), FileData::Chown(other)) => this == other, (FileData::Rename(this), FileData::Rename(other)) => this == other, (FileData::SetXattr(this), FileData::SetXattr(other)) => this == other,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fact/src/event/mod.rs` around lines 538 - 555, The manual PartialEq implementation for FileData is missing the FileData::Chown arm, so identical ownership-change events currently fall through to false. Update the eq method in FileData’s PartialEq match to add a Chown(this), Chown(other) branch that compares the contained values the same way as the other variants, keeping the fallback _ => false only for truly different variants.
🧹 Nitpick comments (1)
fact/src/bpf/checks.rs (1)
34-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider logging the underlying error before treating any load failure as "unsupported."
probe_hookreturnsfalsefor missing programs, wrong program type, and load failures alike, discarding the error fromprog.load(...). This is fine for genuinely unsupported kernels, but it also silently masks a real bug (e.g. a verifier failure inchecks.c) as "capability not supported," making such regressions hard to diagnose.♻️ Suggested diagnostic logging
fn probe_hook(obj: &mut aya::Ebpf, prog_name: &str, hook: &str, btf: &Btf) -> bool { let Some(prog) = obj.program_mut(prog_name) else { return false; }; let Ok(prog): Result<&mut Lsm, _> = prog.try_into() else { return false; }; - prog.load(hook, btf).is_ok() + match prog.load(hook, btf) { + Ok(()) => true, + Err(e) => { + debug!("Failed to probe {hook} support via {prog_name}: {e:#?}"); + false + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fact/src/bpf/checks.rs` around lines 34 - 42, `probe_hook` is swallowing real load errors by returning false for every failure case. Update the function to log the underlying error from `prog.load(hook, btf)` before mapping it to unsupported, while keeping the existing false behavior for missing programs and wrong program types. Use the `probe_hook` function and the `Lsm` load path to locate the change, and make sure the log clearly distinguishes load failures from genuinely unsupported kernels.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fact-ebpf/src/bpf/events.h`:
- Around line 193-199: The ACL copy loop in events.h is always assigning
entry.e_uid.val to args->event->acl.entries[i].e_id, but posix_acl_entry.e_uid
is only valid for ACL_USER and ACL_GROUP. Update the logic in the ACL entry
population path to branch on entry.e_tag and set e_id from e_uid.val only for
those tags; for ACL_USER_OBJ, ACL_GROUP_OBJ, ACL_MASK, and ACL_OTHER, assign
ACL_UNDEFINED_ID instead. Keep the change localized to the ACL event-building
loop so the rest of the field copying in args->event->acl.entries remains
unchanged.
---
Outside diff comments:
In `@fact/src/event/mod.rs`:
- Around line 538-555: The manual PartialEq implementation for FileData is
missing the FileData::Chown arm, so identical ownership-change events currently
fall through to false. Update the eq method in FileData’s PartialEq match to add
a Chown(this), Chown(other) branch that compares the contained values the same
way as the other variants, keeping the fallback _ => false only for truly
different variants.
---
Nitpick comments:
In `@fact/src/bpf/checks.rs`:
- Around line 34-42: `probe_hook` is swallowing real load errors by returning
false for every failure case. Update the function to log the underlying error
from `prog.load(hook, btf)` before mapping it to unsupported, while keeping the
existing false behavior for missing programs and wrong program types. Use the
`probe_hook` function and the `Lsm` load path to locate the change, and make
sure the log clearly distinguishes load failures from genuinely unsupported
kernels.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 5b916de2-f11e-48d7-b345-c3ed9b660223
📒 Files selected for processing (16)
CHANGELOG.mdfact-ebpf/src/bpf/checks.cfact-ebpf/src/bpf/events.hfact-ebpf/src/bpf/main.cfact-ebpf/src/bpf/types.hfact-ebpf/src/lib.rsfact/src/bpf/checks.rsfact/src/bpf/mod.rsfact/src/event/mod.rsfact/src/metrics/kernel_metrics.rstests/event.pytests/server.pytests/test_acl.pytests/test_xattr.pytests/utils.pythird_party/stackrox
| for (unsigned int i = 0; i < FACT_MAX_ACL_ENTRIES && i < count; i++) { | ||
| struct posix_acl_entry entry = {0}; | ||
| bpf_probe_read_kernel(&entry, sizeof(entry), &kacl->a_entries[i]); | ||
| args->event->acl.entries[i].e_tag = entry.e_tag; | ||
| args->event->acl.entries[i].e_perm = entry.e_perm; | ||
| args->event->acl.entries[i].e_id = entry.e_uid.val; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'fact-ebpf/src/bpf/events.h' 'fact-ebpf/src/lib.rs'
wc -l fact-ebpf/src/bpf/events.h fact-ebpf/src/lib.rs
ast-grep outline fact-ebpf/src/bpf/events.h --view expanded
ast-grep outline fact-ebpf/src/lib.rs --view expandedRepository: stackrox/fact
Length of output: 1670
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' fact-ebpf/src/bpf/events.h
printf '\n---- lib.rs ----\n'
sed -n '1,220p' fact-ebpf/src/lib.rsRepository: stackrox/fact
Length of output: 11141
Avoid copying entry.e_uid.val for every ACL tag
posix_acl_entry.e_uid is only valid for ACL_USER / ACL_GROUP; for ACL_USER_OBJ, ACL_GROUP_OBJ, ACL_MASK, and ACL_OTHER this reads uninitialized kernel heap data and emits a bogus e_id. Use ACL_UNDEFINED_ID for the other tags.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fact-ebpf/src/bpf/events.h` around lines 193 - 199, The ACL copy loop in
events.h is always assigning entry.e_uid.val to
args->event->acl.entries[i].e_id, but posix_acl_entry.e_uid is only valid for
ACL_USER and ACL_GROUP. Update the logic in the ACL entry population path to
branch on entry.e_tag and set e_id from e_uid.val only for those tags; for
ACL_USER_OBJ, ACL_GROUP_OBJ, ACL_MASK, and ACL_OTHER, assign ACL_UNDEFINED_ID
instead. Keep the change localized to the ACL event-building loop so the rest of
the field copying in args->event->acl.entries remains unchanged.
Description
Adds ACL tracking via inode_set_acl hook. Due to lack of path info in this hook, it is only possible to monitor inode-tracked files (i.e. those on the host file system) this is similar to how xattrs are tracked.
An extension to the BPF checks.c file has been added, because this hook does not exist on older kernels (where ACLs were applied via xattrs, so we still have coverage on those old kernels, just through a different mechanism.)
Tests factor in this optional nature of the hook, and will only run when we know the kernel supports it.
Relies on stackrox/stackrox#21357
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Tested locally, with setfacl, and the integration tests.
Summary by CodeRabbit
New Features
Bug Fixes
Tests